New sniff to flip ternaries that use compare value on the right side …#2817
Conversation
…of the colon in an empty check
📝 WalkthroughWalkthroughThe pull request systematically replaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.php`:
- Around line 60-75: The current check accepts any simple variable inside
empty() (using $varToken, $variableName and $openParen/$closeParen) but the
auto-fix should only run for parameters; change the guard so before returning
you resolve the surrounding function/closure and verify $variableName is
declared in its parameter list (i.e. find the enclosing T_FUNCTION/T_CLOSURE
token and inspect its parameter tokens) and only allow the sniff/fixer to
proceed when the variable matches one of those parameters; otherwise return
without attempting an auto-fix to avoid introducing undefined variable notices.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
classes/controllers/FrmFormsController.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/fields/FrmFieldCheckbox.phpclasses/models/fields/FrmFieldRadio.phpclasses/models/fields/FrmFieldType.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.phpphpcs-sniffs/Formidable/ruleset.xml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.phpclasses/models/fields/FrmFieldRadio.phpclasses/models/fields/FrmFieldType.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/fields/FrmFieldCheckbox.phpclasses/controllers/FrmFormsController.php
🧬 Code graph analysis (4)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.php (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipNegativeTernarySniff.php (1)
getTokensAsString(297-306)
classes/models/fields/FrmFieldRadio.php (1)
classes/controllers/FrmStylesController.php (2)
FrmStylesController(6-1489)get_style_val(1283-1291)
classes/helpers/FrmFieldsHelper.php (2)
js/src/admin/admin.js (3)
msg(3017-3017)msg(3032-3032)error(9343-9343)classes/models/FrmInstallerSkin.php (1)
error(72-93)
classes/models/fields/FrmFieldCheckbox.php (1)
classes/controllers/FrmStylesController.php (2)
FrmStylesController(6-1489)get_style_val(1283-1291)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (10)
phpcs-sniffs/Formidable/ruleset.xml (1)
35-35: Ruleset update looks good.Line 35 cleanly wires the new sniff into the CodeAnalysis group.
classes/controllers/FrmFormsController.php (1)
2354-2359: Admin bar title update is consistent.Line 2358 aligns with the new truthy-based ternary pattern without altering expected behavior for form names.
classes/models/fields/FrmFieldCheckbox.php (1)
88-90: Alignment lookup change looks good.Line 90 matches the new ternary simplification pattern and should behave as expected for form IDs.
classes/helpers/FrmFieldsHelper.php (1)
475-477: Error message fallback update is fine.Line 476 keeps the fallback logic consistent with the new ternary simplification approach.
classes/models/fields/FrmFieldRadio.php (1)
79-81: Radio alignment lookup change looks good.Line 81 aligns with the new ternary simplification pattern used elsewhere.
classes/models/fields/FrmFieldType.php (1)
369-371: LGTM — clear truthy fallback.Line 370 keeps behavior consistent for the
$nameparameter while aligning with the new ternary style.classes/helpers/FrmXMLHelper.php (2)
197-198: LGTM — parent fallback preserved.The truthy check keeps the same fallback behavior for falsy/zero parent IDs.
2387-2389: LGTM — reply-to “from” fallback preserved.The simplified ternary keeps the intended defaulting to site/admin values when reply-to info is missing.
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.php (2)
31-33: LGTM — correct token registration.Registering
T_EMPTYis appropriate for this sniff.
125-127: The project's PHPCS version constraint (squizlabs/php_codesniffer: ^3.10) ensuresFile::getTokensAsString()is available. This method was introduced in PHPCS 0.0.5 and is stable across all 3.x versions. Three other sniffs in this codebase (SimplifyIfReturnSniff,PreferEscHtmlESniff) already use this API directly without issues, confirming it works reliably in the project's supported environment. WhileFlipNegativeTernarySniffuses a local helper, this appears to be a stylistic choice rather than a requirement for API compatibility.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Get the variable inside empty(). | ||
| $varToken = $phpcsFile->findNext( T_WHITESPACE, $openParen + 1, $closeParen, true ); | ||
|
|
||
| if ( false === $varToken || $tokens[ $varToken ]['code'] !== T_VARIABLE ) { | ||
| return; | ||
| } | ||
|
|
||
| $variableName = $tokens[ $varToken ]['content']; | ||
|
|
||
| // Check if there's only the variable inside empty() (no array access, etc.). | ||
| $nextInParen = $phpcsFile->findNext( T_WHITESPACE, $varToken + 1, $closeParen, true ); | ||
|
|
||
| if ( false !== $nextInParen ) { | ||
| // There's something else inside empty(), skip. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard against non-parameter variables before auto-fix.
The sniff’s docstring states it’s for function parameters, but Line 67+ accepts any simple variable. Auto-fixing non-parameter locals can introduce “undefined variable” notices. Consider restricting fixes to variables that are declared as parameters in the current function/closure.
♻️ Proposed fix
@@
- $variableName = $tokens[ $varToken ]['content'];
+ $variableName = $tokens[ $varToken ]['content'];
+
+ // Only simplify when the variable is a parameter of the current function/closure.
+ if ( ! $this->isFunctionParameter( $phpcsFile, $varToken ) ) {
+ return;
+ }
@@
}
+
+ private function isFunctionParameter( File $phpcsFile, $varToken ) {
+ $tokens = $phpcsFile->getTokens();
+ $conditions = $tokens[ $varToken ]['conditions'] ?? array();
+ $functionPtr = null;
+
+ foreach ( array_reverse( $conditions, true ) as $ptr => $type ) {
+ if ( in_array( $type, array( T_FUNCTION, T_CLOSURE, T_FN ), true ) ) {
+ $functionPtr = $ptr;
+ break;
+ }
+ }
+
+ if ( null === $functionPtr || ! isset( $tokens[ $functionPtr ]['parenthesis_opener'] ) ) {
+ return false;
+ }
+
+ $paramStart = $tokens[ $functionPtr ]['parenthesis_opener'] + 1;
+ $paramEnd = $tokens[ $functionPtr ]['parenthesis_closer'];
+
+ for ( $param = $phpcsFile->findNext( T_VARIABLE, $paramStart, $paramEnd ); false !== $param; $param = $phpcsFile->findNext( T_VARIABLE, $param + 1, $paramEnd ) ) {
+ if ( $tokens[ $param ]['content'] === $tokens[ $varToken ]['content'] ) {
+ return true;
+ }
+ }
+
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the variable inside empty(). | |
| $varToken = $phpcsFile->findNext( T_WHITESPACE, $openParen + 1, $closeParen, true ); | |
| if ( false === $varToken || $tokens[ $varToken ]['code'] !== T_VARIABLE ) { | |
| return; | |
| } | |
| $variableName = $tokens[ $varToken ]['content']; | |
| // Check if there's only the variable inside empty() (no array access, etc.). | |
| $nextInParen = $phpcsFile->findNext( T_WHITESPACE, $varToken + 1, $closeParen, true ); | |
| if ( false !== $nextInParen ) { | |
| // There's something else inside empty(), skip. | |
| return; | |
| } | |
| // Get the variable inside empty(). | |
| $varToken = $phpcsFile->findNext( T_WHITESPACE, $openParen + 1, $closeParen, true ); | |
| if ( false === $varToken || $tokens[ $varToken ]['code'] !== T_VARIABLE ) { | |
| return; | |
| } | |
| $variableName = $tokens[ $varToken ]['content']; | |
| // Only simplify when the variable is a parameter of the current function/closure. | |
| if ( ! $this->isFunctionParameter( $phpcsFile, $varToken ) ) { | |
| return; | |
| } | |
| // Check if there's only the variable inside empty() (no array access, etc.). | |
| $nextInParen = $phpcsFile->findNext( T_WHITESPACE, $varToken + 1, $closeParen, true ); | |
| if ( false !== $nextInParen ) { | |
| // There's something else inside empty(), skip. | |
| return; | |
| } | |
| } | |
| private function isFunctionParameter( File $phpcsFile, $varToken ) { | |
| $tokens = $phpcsFile->getTokens(); | |
| $conditions = $tokens[ $varToken ]['conditions'] ?? array(); | |
| $functionPtr = null; | |
| foreach ( array_reverse( $conditions, true ) as $ptr => $type ) { | |
| if ( in_array( $type, array( T_FUNCTION, T_CLOSURE, T_FN ), true ) ) { | |
| $functionPtr = $ptr; | |
| break; | |
| } | |
| } | |
| if ( null === $functionPtr || ! isset( $tokens[ $functionPtr ]['parenthesis_opener'] ) ) { | |
| return false; | |
| } | |
| $paramStart = $tokens[ $functionPtr ]['parenthesis_opener'] + 1; | |
| $paramEnd = $tokens[ $functionPtr ]['parenthesis_closer']; | |
| for ( $param = $phpcsFile->findNext( T_VARIABLE, $paramStart, $paramEnd ); false !== $param; $param = $phpcsFile->findNext( T_VARIABLE, $param + 1, $paramEnd ) ) { | |
| if ( $tokens[ $param ]['content'] === $tokens[ $varToken ]['content'] ) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyEmptyTernarySniff.php`
around lines 60 - 75, The current check accepts any simple variable inside
empty() (using $varToken, $variableName and $openParen/$closeParen) but the
auto-fix should only run for parameters; change the guard so before returning
you resolve the surrounding function/closure and verify $variableName is
declared in its parameter list (i.e. find the enclosing T_FUNCTION/T_CLOSURE
token and inspect its parameter tokens) and only allow the sniff/fixer to
proceed when the variable matches one of those parameters; otherwise return
without attempting an auto-fix to avoid introducing undefined variable notices.
1 participant
…of the colon in an empty check
This sniff prefers
A ? A : Bover! A ? : B : Aternary logic.Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.